Share default TimelineCredParameters #1376
Conversation
src/analysis/timeline/params.js
Outdated
| @@ -56,3 +57,10 @@ export function paramsFromJSON( | |||
| weights: weightsFromJSON(p.weights), | |||
| }; | |||
| } | |||
|
|
|||
| export function defaultParams(weights?: Weights): TimelineCredParameters { | |||
There was a problem hiding this comment.
While I see your point about only weights being used for overrides. I feel like this approach (selective overrides, and a default parameters function) is somewhat unconventional. Most commonly I see optional arguments to a constructor or factory, which are then merged with the default values.
In some cases the defaults are also exposed, for example in the yaml package, because you may want to reuse your options object for multiple function calls. Though in this case, the TimelineCred class already acts as the way to store and reuse your options.
The bottom line is, I wouldn't make it the responsibility of the caller to know about defaultParams when constructing. Instead make it the constructor / factory's concern to fill in any parameters not provided. And not to be selective based on what's already used in the current code-base. As this convention will make exposing features in a library-style easier later on.
There was a problem hiding this comment.
Good point. I refactored the usage so that all the API endpoints that accept parameters now accept an optional "partial parameters" object. Basically you can provide partial inputs like {weights: myCustomWeights, alpha: myCustomAlpha} and any missing fields get filled in automatically. PTAL.
teamdandelion
force-pushed
the
default-params
branch
from
12cd07a
to
1913468
Compare
teamdandelion
changed the base branch from
timeline-params-module
to
master
teamdandelion
force-pushed
the
default-params
branch
from
1913468
to
72d6ca9
Compare
At present, every place in the codebase that needs TimelineCredParameters constructs them ad-hoc, meaning we don't have any shared defaults across different consumers. This commit adds a new type, `PartialTimelineCredParameters`, which is basically `TimelineCredParameters` with every field marked optional. Callers can then choose to override any fields where they want non-default values. A new internal `partialParams` function promotes these partial parameters to full parameters. All the public interfaces for using params (namely, `TimelineCred.compute` and `TimelineCred.reanalyze`) now accept optional partial params. If the params are not specified, default values are used; if partial params are provided, all the explicitly provided values are used, and unspecified values are initialized to default values. Test plan: A simple unit test was added to ensure that weights overrides work as intended. `git grep "intervalDecay: "` reveals that there are no other explicit parameter constructions in the codebase. All existing unit tests pass.
teamdandelion
force-pushed
the
default-params
branch
from
72d6ca9
to
0c0f22e
Compare
No need; use built-in |
At present, every place in the codebase that needs
TimelineCredParameters constructs them ad-hoc, meaning we don't have any
shared defaults across different consumers.
This commit adds a new type,
PartialTimelineCredParameters, whichis basically
TimelineCredParameterswith every field marked optional.Callers can then choose to override any fields where they want
non-default values. A new internal
partialParamsfunction promotesthese partial parameters to full parameters.
All the public interfaces for using params (namely,
TimelineCred.computeandTimelineCred.reanalyze) now accept optionalpartial params. If the params are not specified, default values are
used; if partial params are provided, all the explicitly provided values
are used, and unspecified values are initialized to default values.
Test plan: A simple unit test was added to ensure that weights overrides
work as intended.
git grep "intervalDecay: "reveals that there are noother explicit parameter constructions in the codebase. All existing
unit tests pass.